Skip to content

refactor(web): clarify function scope and update some function comments - #16532

Merged
ermshiperete merged 3 commits into
masterfrom
refactor/web/funcscope
Sep 8, 2026
Merged

refactor(web): clarify function scope and update some function comments#16532
ermshiperete merged 3 commits into
masterfrom
refactor/web/funcscope

Conversation

@ermshiperete

@ermshiperete ermshiperete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Build-bot: skip build:web
Test-bot: skip

@github-project-automation github-project-automation Bot moved this to Todo in Keyman Sep 4, 2026
@keymanapp-test-bot keymanapp-test-bot Bot added the user-test-missing User tests have not yet been defined for the PR label Sep 4, 2026
@keymanapp-test-bot

keymanapp-test-bot Bot commented Sep 4, 2026

Copy link
Copy Markdown

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

  • Web (no artifacts on BuildLevel "build")
    • KeymanWeb Test Home - build : ✅ all tests passed

@keymanapp-test-bot keymanapp-test-bot Bot added this to the B19S1 milestone Sep 4, 2026
@keymanapp-test-bot keymanapp-test-bot Bot removed the user-test-missing User tests have not yet been defined for the PR label Sep 4, 2026
@ermshiperete
ermshiperete marked this pull request as ready for review September 4, 2026 20:34
Comment on lines 324 to 326
} else {
languageCode == '';
languageCode = '';
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. However, if !keyboardId, we always (line 333) return quickly anyway:

      return {
          keyboard: Promise.resolve(null),
          metadata: null
      }

So it has no impact because it was never used...

It'd be better to move the parameter nullish check to the top of the function and simplify the logic:

  protected prepareKeyboardForActivation(
    keyboardId: string,
    languageCode?: string
  ): {keyboard: Promise<Keyboard>, metadata: KeyboardStub} {

    if(!keyboardId) {
      return {
        keyboard: Promise.resolve(null),
        metadata: null
      }
    }

    // Set default language code
    languageCode ||= '';

    // Check that the saved keyboard is currently registered
    const requestedStub: KeyboardStub = this.keyboardCache.getStub(keyboardId, languageCode);

    if(!requestedStub) {
      const availableStubList = this.keyboardCache.getStubList().map(stub => `${stub.KI}@${stub.KLC}`);
      throw new Error(`No matching stub has been registered for keyboard ${keyboardId}.  Available stubs: ${JSON.stringify(availableStubList)}`);
    }

    // Check if current keyboard matches requested keyboard, but not (necessarily) stub
    if (keyboardId === this.activeKeyboard?.metadata?.id) {
      const {keyboard} = this.activeKeyboard;
      // In this case, the keyboard is loaded; just update the stub.

      return {
        keyboard: Promise.resolve(keyboard),
        metadata: requestedStub
      };
    }

Secondary question, why would we ever call prepareKeyboardForActivation with a nullish keyboardId? That smells buggy in itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Secondary question, why would we ever call prepareKeyboardForActivation with a nullish keyboardId? That smells buggy in itself.

KeymanEngine.removeKeyboards calls activateKeyboard (which then calls prepareKeyboardForActivation) with an empty string: this.contextManager.activateKeyboard('', '');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Base automatically changed from chore/web/typesInTest to master September 7, 2026 15:05
@ermshiperete
ermshiperete force-pushed the refactor/web/funcscope branch from 5640599 to 04a5cd1 Compare September 7, 2026 15:05
Co-authored-by: Marc Durdin <marc@durdin.net>
keyboard: Promise.resolve(keyboard),
metadata: requestedStub
};
} else {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the changes in this file are just removing the else (since the if returns), and adjusting the indentation. You might want to ignore the whitespace changes when reviewing.

@ermshiperete
ermshiperete dismissed mcdurdin’s stale review September 7, 2026 16:38

Requesting re-review

@mcdurdin mcdurdin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I think we should refactor the callers to prepareKeyboardForActivation() such that they never call with a nullish keyboardId because it seems like that is really out of scope for the purpose of this function, but it's good to keep this refactor small.

@ermshiperete
ermshiperete merged commit 7b1e2eb into master Sep 8, 2026
8 checks passed
@ermshiperete
ermshiperete deleted the refactor/web/funcscope branch September 8, 2026 09:56
@github-project-automation github-project-automation Bot moved this from Todo to Done in Keyman Sep 8, 2026
@keyman-server

Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 19.0.282-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants